Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REFACTOR: Improve try/except statements in _get_design_mesh_regions and _get_design_mesh_operations #4636

Merged
merged 5 commits into from
May 10, 2024

Conversation

lorenzovecchietti
Copy link
Collaborator

Close #4632.
Current implementation of _get_design_mesh_regions and _get_design_mesh_operations lead to errors being logged, I removed this improving try/except statements

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@lorenzovecchietti lorenzovecchietti changed the title Get mesh op reg error handling REFAC: Improve try/except statements in _get_design_mesh_regions and _get_design_mesh_operations May 6, 2024
@lorenzovecchietti lorenzovecchietti changed the title REFAC: Improve try/except statements in _get_design_mesh_regions and _get_design_mesh_operations REFAC: Improve try/except statements in _get_design_mesh_regions and _get_design_mesh_operations May 6, 2024
@lorenzovecchietti lorenzovecchietti changed the title REFAC: Improve try/except statements in _get_design_mesh_regions and _get_design_mesh_operations REFACTOR: Improve try/except statements in _get_design_mesh_regions and _get_design_mesh_operations May 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.82%. Comparing base (34efaf7) to head (f3f7f5e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4636      +/-   ##
==========================================
- Coverage   80.84%   80.82%   -0.02%     
==========================================
  Files         119      119              
  Lines       54531    54537       +6     
==========================================
- Hits        44084    44081       -3     
- Misses      10447    10456       +9     

@lorenzovecchietti lorenzovecchietti marked this pull request as ready for review May 6, 2024 14:13
Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to know why the TypeError is raised ? If not I would rather use self._app.logger.debug("Failed to get design mesh operations") for every exception.
For the Keyerror handling, I would also use the previous log as it is considered bad practice to use a try: ... except: pass pattern. Tools that we'll use for vulnerability checking would report such things as a vulnerability.

@lorenzovecchietti
Copy link
Collaborator Author

Are you able to know why the TypeError is raised ?

TypeError gets raised because, when the file is not saved, self._app.design_properties is None, so it cannot be accessed like a dictionary. (Mesh regions/operations are unfortunately one of the few things we still need to parse from the file)

For the Keyerror handling, I would also use the previous log as it is considered bad practice to use a try: ... except: pass pattern.
I can think of something else to log? Because when KeyError is raised, we know we have a design dictionary parsed, but it simply doesn't have the keys needed to define a mesh region/mesh operations.

In both cases, the user was previously seeing an error being logged when the code was actually working as intended (empty mesh region/empty mesh operation list) all the time he was accessing the mesh class with a new proj/a project without any region.

I can try to rewrite everything from scratch, but I wouldn't leave an error being raised and logged when the code is working properly (and in addition given that the error comes from an internal function) as this could throw off a lot of beginner/intermediate users.

@lorenzovecchietti lorenzovecchietti marked this pull request as draft May 6, 2024 14:57
@SMoraisAnsys
Copy link
Collaborator

Are you able to know why the TypeError is raised ?

TypeError gets raised because, when the file is not saved, self._app.design_properties is None, so it cannot be accessed like a dictionary. (Mesh regions/operations are unfortunately one of the few things we still need to parse from the file)

For the Keyerror handling, I would also use the previous log as it is considered bad practice to use a try: ... except: pass pattern.
I can think of something else to log? Because when KeyError is raised, we know we have a design dictionary parsed, but it simply doesn't have the keys needed to define a mesh region/mesh operations.

In both cases, the user was previously seeing an error being logged when the code was actually working as intended (empty mesh region/empty mesh operation list) all the time he was accessing the mesh class with a new proj/a project without any region.

I can try to rewrite everything from scratch, but I wouldn't leave an error being raised and logged when the code is working properly (and in addition given that the error comes from an internal function) as this could throw off a lot of beginner/intermediate users.

When I was speaking about the previous log I wanted to refer to the one I did type. Sorry for not being clear enough. What you did is great and the right thing to do. We should just avoid using the try: except: pass pattern.
Logging something in debug wouldn't be caught be users AFAIK and it could be helpful for debugging

Copy link
Member

@Samuelopez-ansys Samuelopez-ansys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Samuelopez-ansys Samuelopez-ansys enabled auto-merge (squash) May 10, 2024 06:36
@Samuelopez-ansys Samuelopez-ansys merged commit 783657f into main May 10, 2024
31 checks passed
@Samuelopez-ansys Samuelopez-ansys deleted the get_mesh_op_reg_error_handling branch May 10, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error is logged by _get_design_mesh_regions and _get_design_mesh_operations
4 participants